❌ Deploy Preview for volto failed. Why did it fail? →

Passing run #7234 ↗︎

0

14

0

0

0

Details:

Review all test suite changes for PR #3592 ↗︎

Link view now working, Image view still has issues with the download link which is not working (see this example)

During the Volto Team meeting we agreed that we would have to put in place a whole round of Cypress tests pointing to a deployment using this. I can imagine that in the future will be quite easy to break the whole feature if one does not have in mind it.

I think that could be time consuming, but might not have much difficulty. What do you think?

@mamico @giuliaghisini could you share the reverse proxy config you have on such deployments?

We should add some documentation about how to setup such a deployment.
Also, I'll try to write a traefik config for testing it properly in CI.

@sneridagh our deployment config is a bit complicated because there are several urls and frontend names, and it's made in varnish and not in nginx/apache.

The conf itself for using this branch is easy. Here is an example for nginx:

upstream backend {
  server host.docker.internal:8080;
}
upstream frontend {
  server host.docker.internal:3000;
}

server {
  listen 80  default_server;
  server_name  localhost;

  location ~ /foo/\+\+api\+\+($|/) {
      rewrite ^/foo/\+\+api\+\+($|/.*) /VirtualHostBase/http/${server_name}/Plone/++api++/VirtualHostRoot/_vh_foo$1 break;
      proxy_pass http://backend;
  }


  location ~ /foo($|/) {
    
    # the standard nginx conf
    ...

    proxy_pass http://frontend;
  }
}

@cekk One other thing is important to document, the prefix path /foo corresponds to the Plone root, not a /foo subfolder in Plone.

Yes, /foo points to the root of Plone site

@cekk To make it work I had to launch Volto with:

RAZZLE_PREFIX_PATH=/foo RAZZLE_API_PATH=http://localhost/foo yarn start

I would have expect that given a RAZZLE_PREFIX_PATH, the other would have adjusted automatically (as seamless mode promises). I am doing something wrong? because given a look at the code, it seems it should, right?

@cekk Forget the question, I'm still asleep. 😅

Added tentative tests: #3719 see comments.

I would like to take this forward. Locally looks good. I will checkout this branch on one of our projects first and see if I find some issues.

State of this PR:

This PR is based on basename react-router prop which adds an initial entry to browser "history" stack and allows all links to be prefixed with basename by react-router. The static assets like images need to be explicitly prefixed. I amended an express-middleware which re-directs to app base path on initial load.

There is another approach in this PR, which uses a store enhancer middleware to prefix all the router paths(amending history accordingly) and modified flattenToAppUrl method.

What's left are the cypress tests #3719 which should also account for prefix path in the URLs. I will try to have a look into them.

I personally like the basename approach and let react-router handle the prefixes. However, we need to think about the non-router links and static assets. What do you think @sneridagh @davisagli @tiberiuichim @pnicolli ?

@giuliaghisini @nileshgulia1 @sneridagh any chance we have this feature in master?

I updated the branch and run:

RAZZLE_PREFIX_PATH=/my-prefix yarn start

Then I got the error:

ValidationError: Invalid options object. Dev Server has been initialized using an options object that does not match the API schema.
 - options has an unknown property 'publicPath'. These properties are valid:
   object { allowedHosts?, bonjour?, client?, compress?, devMiddleware?, headers?, historyApiFallback?, host?, hot?, http2?, https?, ipc?, liveReload?, magicHtml?, onAfterSetupMiddleware?, onBeforeSetupMiddleware?, onListening?, open?, port?, proxy?, server?, setupExitSignals?, setupMiddlewares?, static?, watchFiles?, webSocketServer? }
    at validate (/home/user/git/volto/node_modules/webpack-dev-server/node_modules/schema-utils/dist/validate.js:115:11)
    at new Server (/home/user/git/volto/node_modules/webpack-dev-server/lib/Server.js:231:5)
    at new razzleDevServer (/home/user/git/volto/node_modules/razzle/config/razzleDevServer.js:10:5)
    at /home/user/git/volto/node_modules/razzle/scripts/start.js:181:33
    at processTicksAndRejections (node:internal/process/task_queues:96:5)

I'll take a look at it.

@wesleybl it's due to the changes in razzle.config.js

@tiberiuichim I fixed this in: f011df6

@wesleybl I agree that we should have a basename set, that is the starting point in the PR, it takes care of prefixing for us. We need to identify the parts where the prefix is manually needed. For me, focus is more on trying to avoid affecting the future development of volto. Or if we should really introduce env vars starting with RAZZLE_ now. There are lof of efforts that uses another build system and maybe in future we might shift there.
@wesleybl If you plan to attend PloneConf, then let's discuss this in detail there during sprints or in general.

We need to identify the parts where the prefix is manually needed.

@nileshgulia1 basically, we need to manually prefix in the HTML tags a and img, since React Router does not influence these tags. That's why we need the addPrefixPath function. In this PR, we have already replaced the use of the img tag with the Image component, and we use addPrefixPath inside the Image component. Volto also practically does not use the a tag for links. It doesn't even make sense to use the a tag in React. The only situation in which the a tag is necessary is for file downloads. But file downloads are encapsulated in the UniversalLink component and there we use addPrefixPath.

In short, if we use the UniversalLink, Link and Image components, everything will be fine.

I'd love to talk to you as well in person

@sneridagh I sent you an email.

I'd love to talk to you as well in person

@sneridagh I sent you an email.

I'll be happy to join as well. Just ping me in discord.

@nileshgulia1 @sneridagh I documented in PLIP situations where it is necessary to manually prefix, when using react-router's basename. Is basically what is being done in this PR.

I wonder if this or similar could be included..

#3592 (comment)

Anyway, I hope this will merged soon! :- )

I wonder if this or similar could be included..

#3592 (comment)

@teekuningas In our use case, we want cookies to be shared. But perhaps your suggestion could be a setting.

I wonder if this or similar could be included..
#3592 (comment)

@teekuningas In our use case, we want cookies to be shared. But perhaps your suggestion could be a setting.

Interesting, I wonder if that would work for us too.. anyway, a setting would be enough, yes.

@wesleybl @teekuningas I talked to @sneridagh during PloneConf, and we agreed that we should rewrite the PR. We should extract the useful parts from it and possibly rewrite the PLIP to ensure it works with the current setup and can be easily upgraded when we move to Seven. What do you think? I agree to provide support maybe we should get in touch soon and target a sprint for it?

I talked to @sneridagh during PloneConf, and we agreed that we should rewrite the PR

@nileshgulia1 That's exactly what I started doing with #6754. But instead of doing it all in one big PR, I think it's easier to split it up into multiple PRs.